Feature/react query migration#1629
Feature/react query migration#1629jesusbalderramawgu wants to merge 22 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @jesusbalderramawgu! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
for some reason the lint is failing with the following files webpack.prod.config.js, .eslintrc.js and jest.config.js. |
|
assign me |
tsconfig.json
Outdated
| "outDir": "dist" | ||
| }, | ||
| "include": ["*.js", ".eslintrc.js", "src/**/*", "plugins/**/*", "jest.config.ts"], | ||
| "exclude": ["*.js", ".eslintrc.js", "dist", "node_modules"] |
There was a problem hiding this comment.
| "exclude": ["*.js", ".eslintrc.js", "dist", "node_modules"] | |
| "exclude": ["dist", "node_modules"] |
This fixes the lint problems.
There was a problem hiding this comment.
thank you Jacob, this fixed the issue
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1629 +/- ##
==========================================
+ Coverage 87.57% 91.22% +3.65%
==========================================
Files 125 108 -17
Lines 2309 2347 +38
Branches 648 667 +19
==========================================
+ Hits 2022 2141 +119
+ Misses 278 199 -79
+ Partials 9 7 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0428d62 to
6b4d629
Compare
arbrandes
left a comment
There was a problem hiding this comment.
Thank you for this work, Jesus! I like that tests are passing and things generally work. But beyond the inline comments and change requests I made, there are a few things to address:
1. Error responses don't show
The error responses don't seem to be rendered for the user. Take this one in master, for example:
In this branch the error response is simply not shown.
2. Some patterns are not consistent
I focused mostly on the different apiHooks.ts files: each one seems to do things slightly differently. While there is no Open edX guideline for useMutation() calls, we should pick one pattern and stick with it at least across each repository. It makes things easier to maintain. See the inline comments.
It's possible this applies elsewhere and I didn't catch it. But please keep the concept in mind.
3. Remove dead code
Please remove all dead code - basically, everything Redux - in this PR. There isn't much advantage in creating a separate PR for that and leaving a bunch of commented-out stuff in. It's fine if it's a separate commit, though.
4. Types
I'm not going to block the PR on it because the focus is on react-query, not Typescript, but wherever possible it would be much better if we replaced any with actual types, even if it's just the primitive ones. At the very least we should create a follow-up ticket to clean those up.
Again, thanks for the hard work!
| setBannerEmail(emailUsed); | ||
| setFormErrors(''); | ||
| }, | ||
| onError: () => { |
There was a problem hiding this comment.
You need to declare the error parameter:
| onError: () => { | |
| onError: (error) => { |
| error: string | null; | ||
| setError: (error: string | null) => void; | ||
| isLoading: boolean; | ||
| setIsLoading: (isLoading: boolean) => void; |
There was a problem hiding this comment.
The implementation of the context value below is completely different from this type. Please fix it.
src/logistration/Logistration.jsx
Outdated
There was a problem hiding this comment.
I know this was here before your PR, but this is as good an opportunity to fix it as any. If we don't add a dependency array, this will request a new CSRF token on every render:
| }, []); |
There was a problem hiding this comment.
I didn't see this, good catch. Fixed!
src/register/RegistrationPage.jsx
Outdated
| }); | ||
|
|
||
| const registrationErrorCode = registrationError?.errorCode || backendRegistrationError?.errorCode; | ||
| const submitState = registrationMutation.isLoading ? PENDING_STATE : DEFAULT_STATE; |
There was a problem hiding this comment.
Are you sure we don't mean isPending, here? See the react-query v5 migration blurb. Just checking.
| const submitState = registrationMutation.isLoading ? PENDING_STATE : DEFAULT_STATE; | |
| const submitState = registrationMutation.isPending ? PENDING_STATE : DEFAULT_STATE; |
There was a problem hiding this comment.
yeah you are right I changed this
src/login/api/loginApi.js
Outdated
| @@ -0,0 +1,35 @@ | |||
| // TODO: Delete this file | |||
There was a problem hiding this comment.
Please go ahead and delete any dead code.
src/register/data/api.hook.ts
Outdated
| @@ -0,0 +1,53 @@ | |||
| import { camelCaseObject } from '@edx/frontend-platform'; | |||
There was a problem hiding this comment.
Please name this file with the same pattern as the other ones, apiHook.ts.
src/login/data/apiHook.ts
Outdated
| mutationFn: async (loginData: LoginData) => { | ||
| try { | ||
| return await login(loginData); | ||
| } catch (error) { |
There was a problem hiding this comment.
Any particular reason you're not using the onError pattern (which you do use elsewhere)?
src/register/data/api.hook.ts
Outdated
|
|
||
| const useRegistration = (options = {}) => useMutation({ | ||
| mutationFn: (registrationPayload) => registerNewUserApi(registrationPayload), | ||
| onSuccess: (data) => { |
There was a problem hiding this comment.
Here you are using onSuccess and onError outside the mutationFn, which is different to how you do it elsewhere. We should settle on a pattern and use it consistently everywhere, for ease of maintenance.
src/register/data/api.hook.ts
Outdated
| import { getFieldsValidations, registerNewUserApi } from './api.ts'; | ||
| import { INTERNAL_SERVER_ERROR } from './constants'; | ||
|
|
||
| const useRegistration = (options = {}) => useMutation({ |
There was a problem hiding this comment.
Please add a type to the options, like you do for progressive-profiling.
src/login/tests/LoginPage.test.jsx
Outdated
| )); | ||
| expect(store.dispatch).not.toHaveBeenCalledWith(loginRequest({})); | ||
| fireEvent.click(screen.getByRole('button', { name: /sign in/i })); | ||
| expect(props.loginRequest).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
We shouldn't be testing the Redux stuff, anymore.
|
Ah, and I forgot one thing: please try not to reduce coverage, both at the diff level and project level. |
thanks for this comment, yeah I fixed the existing ones and added new ones for the new code, but also I deleted the ones related to redux. I will check this tomorrow and I will add more tests to get the correct coverage |
…ms, renaming of files
bb02894 to
d999091
Compare
arbrandes
left a comment
There was a problem hiding this comment.
Ok, I can confirm the error handling bug is fixed, thanks! Thank you for addressing my comments, otherwise: the hooks refactor looks great!
There are still changes to make, however. For instance:
- Please don't disable the exhaustive-deps linting rules. I know there are some stable objects that wouldn't change, but better to be explicit than implicit. If the dependencies are too big, then the contexts need to be split up.
package.json
Outdated
There was a problem hiding this comment.
Please remove the redux dependencies as well. There might be others I didn't mark: if so, remove them too.
src/MainApp.jsx
Outdated
|
|
||
| registerIcons(); | ||
|
|
||
| const queryClient = new QueryClient(); |
There was a problem hiding this comment.
Auth-related mutations (login, registration) probably should not retry on failure — a failed login retried 3 times would be confusing. Maybe something like:
| const queryClient = new QueryClient(); | |
| const queryClient = new QueryClient({ | |
| defaultOptions: { | |
| mutations: { retry: false }, | |
| }, | |
| }); |
| setRegistrationResult, | ||
| setBackendCountryCode, | ||
| setRegistrationError, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
It should not be necessary to disable any eslint warnings. If the context is too big, let's split it up. Let's try splitting into at least RegistrationFormContext and RegistrationValidationContext.
There was a problem hiding this comment.
I agree with this, I'm just concern splitting this into two contexts right now, basically I just moved the redux state into react context, didn't make any refactor. I think we can make a refactor later maybe when I work on the frontend-base update. what do you think?
by now what I did was to do a mini refactor to remove some properties that are not required in the context and move it into the local state. Also I have the useReducer and useCallback getting stable references avoiding unnecessary re renders, now it is shorter and removed the eslint warning. Looks better
| const location = useLocation(); | ||
|
|
||
| const registrationResponse = location.state?.registrationResult; | ||
| // const registrationResponse = location.state?.registrationResult; |
There was a problem hiding this comment.
There's still some commented out stuff.
| const userId = location.state?.userId; | ||
|
|
||
| const userCountry = useSelector((state) => state.register.backendCountryCode); | ||
| // const userCountry = useSelector((state) => state.register.backendCountryCode); |
There was a problem hiding this comment.
Remove reference to redux.
tsconfig.json
Outdated
| { | ||
| "extends": "@edx/typescript-config", | ||
| "compilerOptions": { | ||
| "baseUrl": "./src", |
There was a problem hiding this comment.
Don't use baseUrl, please. Use rootDir instead.
src/login/LoginPage.jsx
Outdated
| setErrorCode({ | ||
| type: INVALID_FORM, | ||
| count: prevState.count + 1, | ||
| count: errorCode.count + 1, |
There was a problem hiding this comment.
This is not right. You cannot use errorCode directly, here. You're basically using a stale reference. You should, instead:
setErrorCode(prev => ({
type: INVALID_FORM,
count: prev.count + 1,
context: {},
}));
src/login/LoginPage.jsx
Outdated
| onError: (formattedError) => { | ||
| setErrorCode({ | ||
| type: formattedError.type, | ||
| count: errorCode.count + 1, |
There was a problem hiding this comment.
Same problem as on line 173. errorCode is a stale reference. Do something like:
setErrorCode(prev => ({
type: formattedError.type,
count: prev.count + 1,
context: formattedError.context,
}));
Thank you Adolfo for your comments, I addressed the changes that you requested also the code coverage was fixed. |
arbrandes
left a comment
There was a problem hiding this comment.
Thanks for bearing with me! I found a few more things to change.
| setValidationsFailure, | ||
| validationApiRateLimited, | ||
| clearRegistrationBackendError, | ||
| } = useRegisterContext(); |
There was a problem hiding this comment.
The context is not always available. For example, from ResetPasswordPage. If somebody clicks on a /password_reset_confirm/ link, this will throw an exception. The tests are only passing because they wrap the page with the provider, but this is not the case in the app itself.
In other words... probably use RegisterProvider in ResetPasswordPage.
There was a problem hiding this comment.
yeah, it should be there, for some reason I deleted it. now I added the provider again.
But in the future we should refactor this PasswordField I guess to avoid dependency with this context and make it more generic
| const { formatMessage } = useIntl(); | ||
| const isExtraSmall = useMediaQuery({ maxWidth: breakpoints.extraSmall.maxWidth - 1 }); | ||
| const { | ||
| registrationResult, |
There was a problem hiding this comment.
registrationResult doesn't exist in useRegisterContext(). The user will always be redirected to the dashboard, below.
ProgressiveProfiling gets this from the route (via useLocation()). Maybe do the same here?
There was a problem hiding this comment.
in this case I brought the registrationResult back to the register context, because it takes the url when a user registers and other data. that's why it was in the context in first place, I missed this last night.
Maybe this is a refactor for the next step
| if (status === TOKEN_STATE.PENDING) { | ||
| if (token) { | ||
| props.validateToken(token); | ||
| validateResetToken(token, { |
There was a problem hiding this comment.
You can't call this directly in the render function. It's going to be called on every render. This needs to be moved into a useEffect with the [token] as a dependency.
(There's some other similar weirdness on this page, but most of it was already there. This one is worth fixing now because it's using a react-query mutation.)
There was a problem hiding this comment.
didn't notice, that it was directly. I made the change, thank you
| }; | ||
|
|
||
| Logistration.defaultProps = { | ||
| selectedPage: REGISTER_PAGE, |
There was a problem hiding this comment.
Without this default, the register page in MainApp.jsx needs an explicit selectedPage, too.
src/MainApp.jsx
Outdated
| <UnAuthOnlyRoute><Logistration selectedPage={LOGIN_PAGE} /></UnAuthOnlyRoute> | ||
| } | ||
| /> | ||
| <Route path={REGISTER_PAGE} element={<UnAuthOnlyRoute><Logistration /></UnAuthOnlyRoute>} /> |
There was a problem hiding this comment.
Needs an explicit selected page now that the default is gone:
| <Route path={REGISTER_PAGE} element={<UnAuthOnlyRoute><Logistration /></UnAuthOnlyRoute>} /> | |
| <Route path={REGISTER_PAGE} element={<UnAuthOnlyRoute><Logistration selectedPage={REGISTER_PAGE} /></UnAuthOnlyRoute>} /> |
| // Error constants | ||
| export const THIRD_PARTY_AUTH_ERROR = 'third-party-auth-error'; | ||
|
|
||
| const useThirdPartyAuthHook = () => useMutation({ |
There was a problem hiding this comment.
Why use a mutation for a GET request?
There was a problem hiding this comment.
you are right, I made the change
src/forgot-password/data/apiHook.ts
Outdated
| forgotPassword(email) | ||
| ), | ||
| onSuccess: (data: ForgotPasswordResult, email: string) => { | ||
| logInfo(`Forgot password email sent to ${email}`); |
There was a problem hiding this comment.
We shouldn't be logging user's emails. Was the code doing this before?
There was a problem hiding this comment.
this was my mistake I removed it
| @@ -207,18 +245,4 @@ ResetPasswordPage.defaultProps = { | |||
| errorMsg: null, | |||
| }; | |||
There was a problem hiding this comment.
These aren't needed anymore, right?
| status: null, | ||
| submitState: DEFAULT_STATE, | ||
| }; | ||
| ForgotPasswordPage.defaultProps = {}; |
There was a problem hiding this comment.
We don't need defaultProps at all, no?
| status: PropTypes.string, | ||
| submitState: PropTypes.string, | ||
| }; | ||
| ForgotPasswordPage.propTypes = {}; |
There was a problem hiding this comment.
I don't think we need this, anymore.
Thank you Adolfo, I addressed the other comments |



Description
PR to migrate this repository from Redux to React-query and React context. It closes the issue
What changed?
React context was added for client side state maintaining backward compatibility with the currents components, while Redux was changed for react query for server state management.
As it was mentioned above, the functionality was kept with changes required in some components to make it work with the new hooks. The tests were updated as well after the migration.
How Has This Been Tested?
Note:
Even though the unit tests pass, there are some things that I couldn't test manually, such as
if someone knows how to test this, please let me know.